feat: verifies namespace hash format to prevent panic in validateSiblingsNamespaceOrder #185
Conversation
Codecov Report
@@ Coverage Diff @@
## master #185 +/- ##
==========================================
+ Coverage 95.66% 95.69% +0.02%
==========================================
Files 5 5
Lines 554 557 +3
==========================================
+ Hits 530 533 +3
Misses 14 14
Partials 10 10
|
staheri14
changed the title
staheri14
changed the title
| // the minimum namespace ID of the right sibling. It returns ErrUnorderedSiblings error if the check fails. Note that the function assumes | ||
| // that the left and right nodes are in correct format, i.e., they are | ||
| // namespaced hash values. Otherwise, it panics. | ||
| // the minimum namespace ID of the right sibling. It returns ErrUnorderedSiblings error if the check fails. |
There was a problem hiding this comment.
[unrelated] since a namespace now consists of a version and an ID, "namespace" may be clearer than "namespace ID". Thoughts on renaming usage of "namespace ID" to "namespace" in this repo so that the term matches its usage in celestia-app. If yes, I can create a new issue.
There was a problem hiding this comment.
I agree that the renaming is a good idea, as long as we're consistent with the terms we use. Regarding the namespace version, would it be something that the nmt lib needs to handle, or is it transparent and won't impact implementation? As far as I understand, the only change is the size of the namespace, and any further parsing of the namespace byte slice would depend on the specific logic of the application, correct?
There was a problem hiding this comment.
or is it transparent and won't impact implementation?
Won't impact implementation. The NMT lib is already used by celestia-app so no further changes are necessary to support namespaces with versions.
As far as I understand, the only change is the size of the namespace, and any further parsing of the namespace byte slice would depend on the specific logic of the application, correct?
Correct.
Overview
Closes #138
Checklist